pull: Do GPG verify commit objects when using deltas

The fact that we weren't doing this is at best an oversight, and
for some deployment models a security vulnerability.  Having both
`gpg-verify` and `gpg-verify-summary` shows that we were intending
them to be orthogonal/independent.

Lately I've been advocating moving towards pinned TLS instead of
gpg-signed summaries, and if we follow that path, performing GPG
verification of commit objects even if using deltas is more important,
as it provides an at-rest verifiable authenticity and integrity
mechanism.

Content providers which are signing their summary files and/or using
TLS (particularly pinned TLS) for transport should treat this as a
nice-to-have.  However, for providers which are serving content over
plain HTTP and relying on GPG, this is a critical update.

Closes: https://github.com/ostreedev/ostree/issues/517
Closes: #589
Approved by: jlebon

diff --git a/src/libostree/ostree-repo-pull.c b/src/libostree/ostree-repo-pull.c
index 943e170659bba245b65ace9ad200db1bf7df2f31..8c18052f1835d214e83d2fdd47055b83fba39f9a 100644 (file)
--- a/src/libostree/ostree-repo-pull.c
+++ b/src/libostree/ostree-repo-pull.c
@@ -1027,6 +1027,57 @@ static_deltapart_fetch_on_complete (GObject           *object,
     fetch_static_delta_data_free (fetch_data);
 }
 
+static gboolean
+process_verify_result (OtPullData            *pull_data,
+                       const char            *checksum,
+                       OstreeGpgVerifyResult *result,
+                       GError               **error)
+{
+  if (result == NULL)
+    return FALSE;
+
+  /* Allow callers to output the results immediately. */
+  g_signal_emit_by_name (pull_data->repo,
+                         "gpg-verify-result",
+                         checksum, result);
+
+  return ostree_gpg_verify_result_require_valid_signature (result, error);
+}
+
+static gboolean
+gpg_verify_unwritten_commit (OtPullData         *pull_data,
+                             const char         *checksum,
+                             GVariant           *commit,
+                             GVariant           *detached_metadata,
+                             GCancellable       *cancellable,
+                             GError            **error)
+{
+  if (pull_data->gpg_verify)
+    {
+      glnx_unref_object OstreeGpgVerifyResult *result = NULL;
+      g_autoptr(GBytes) signed_data = g_variant_get_data_as_bytes (commit);
+
+      if (!detached_metadata)
+        {
+          g_set_error_literal (error, G_IO_ERROR, G_IO_ERROR_FAILED,
+                               "No detached metadata found for GPG verification");
+          return FALSE;
+        }
+
+      result = _ostree_repo_gpg_verify_with_metadata (pull_data->repo,
+                                                      signed_data,
+                                                      detached_metadata,
+                                                      pull_data->remote_name,
+                                                      NULL, NULL,
+                                                      cancellable,
+                                                      error);
+      if (!process_verify_result (pull_data, checksum, result, error))
+        return FALSE;
+    }
+
+  return TRUE;
+}
+
 static gboolean
 scan_commit_object (OtPullData         *pull_data,
                     const char         *checksum,
@@ -1075,16 +1126,7 @@ scan_commit_object (OtPullData         *pull_data,
                                                      pull_data->remote_name,
                                                      cancellable,
                                                      error);
-
-      if (result == NULL)
-        goto out;
-
-      /* Allow callers to output the results immediately. */
-      g_signal_emit_by_name (pull_data->repo,
-                             "gpg-verify-result",
-                             checksum, result);
-
-      if (!ostree_gpg_verify_result_require_valid_signature (result, error))
+      if (!process_verify_result (pull_data, checksum, result, error))
         goto out;
     }
 
@@ -1563,7 +1605,6 @@ process_one_static_delta (OtPullData   *pull_data,
   {
     g_autoptr(GVariant) to_csum_v = NULL;
     g_autofree char *to_checksum = NULL;
-    g_autoptr(GVariant) to_commit = NULL;
     gboolean have_to_commit;
 
     to_csum_v = g_variant_get_child_value (delta_superblock, 3);
@@ -1578,10 +1619,16 @@ process_one_static_delta (OtPullData   *pull_data,
     if (!have_to_commit)
       {
         FetchObjectData *fetch_data;
+        g_autoptr(GVariant) to_commit = g_variant_get_child_value (delta_superblock, 4);
         g_autofree char *detached_path = _ostree_get_relative_static_delta_path (from_revision, to_revision, "commitmeta");
         g_autoptr(GVariant) detached_data = NULL;
 
         detached_data = g_variant_lookup_value (metadata, detached_path, G_VARIANT_TYPE("a{sv}"));
+
+        if (!gpg_verify_unwritten_commit (pull_data, to_revision, to_commit, detached_data,
+                                          cancellable, error))
+          goto out;
+
         if (detached_data && !ostree_repo_write_commit_detached_metadata (pull_data->repo,
                                                                           to_revision,
                                                                           detached_data,
@@ -1595,8 +1642,6 @@ process_one_static_delta (OtPullData   *pull_data,
         fetch_data->is_detached_meta = FALSE;
         fetch_data->object_is_stored = FALSE;
 
-        to_commit = g_variant_get_child_value (delta_superblock, 4);
-
         ostree_repo_write_metadata_async (pull_data->repo, OSTREE_OBJECT_TYPE_COMMIT, to_checksum,
                                           to_commit,
                                           pull_data->cancellable,

diff --git a/tests/test-remote-gpg-import.sh b/tests/test-remote-gpg-import.sh
index 4dbe7fd9d06a103868f2ebe2c0744415a4dfe387..4429d8bcd2999e7b5e5d5110e6e260e333fe4587 100755 (executable)
--- a/tests/test-remote-gpg-import.sh
+++ b/tests/test-remote-gpg-import.sh
@@ -26,7 +26,7 @@ unset OSTREE_GPG_HOME
 
 setup_fake_remote_repo1 "archive-z2"
 
-echo "1..2"
+echo "1..4"
 
 cd ${test_tmpdir}
 mkdir repo
@@ -171,4 +171,39 @@ fi
 assert_file_has_content err.txt "GPG signatures found, but none are in trusted keyring"
 
 echo "ok"
+
+# Test deltas with signed commits; this test is a bit
+# weird here but this file has separate per-remote keys.
+cd ${test_tmpdir}
+rm repo/refs/remotes/* -rf
+${OSTREE} prune --refs-only
+echo $(date) > workdir/testfile-for-deltas-1
+# Sign with keyid 1 for first commit
+${CMD_PREFIX} ostree  --repo=${test_tmpdir}/ostree-srv/gnomerepo commit -b main --gpg-sign ${TEST_GPG_KEYID_1} --gpg-homedir ${test_tmpdir}/gpghome workdir
+prevrev=$(${CMD_PREFIX} ostree --repo=${test_tmpdir}/ostree-srv/gnomerepo rev-parse main)
+# Pull the previous revision
+${OSTREE} pull R1:main
+assert_streq $(${OSTREE} rev-parse R1:main) ${prevrev}
+# Sign with keyid 2, but use remote r1
+echo $(date) > workdir/testfile-for-deltas-2
+${CMD_PREFIX} ostree  --repo=${test_tmpdir}/ostree-srv/gnomerepo commit -b main --gpg-sign ${TEST_GPG_KEYID_2} --gpg-homedir ${test_tmpdir}/gpghome workdir
+${CMD_PREFIX} ostree --repo=${test_tmpdir}/ostree-srv/gnomerepo static-delta generate main
+# Summary is signed with key1
+${CMD_PREFIX} ostree --repo=${test_tmpdir}/ostree-srv/gnomerepo summary -u --gpg-sign ${TEST_GPG_KEYID_1} --gpg-homedir ${test_tmpdir}/gpghome
+newrev=$(${CMD_PREFIX} ostree --repo=${test_tmpdir}/ostree-srv/gnomerepo rev-parse main)
+if ${OSTREE} pull --require-static-deltas R1:main 2>err.txt; then
+    assert_not_reached "Unexpectedly succeeded at pulling commit signed with untrusted key"
+fi
+assert_file_has_content err.txt "GPG signatures found, but none are in trusted keyring"
+
+echo "ok gpg untrusted signed commit for delta upgrades"
+
+${CMD_PREFIX} ostree  --repo=${test_tmpdir}/ostree-srv/gnomerepo reset main{,^}
+${CMD_PREFIX} ostree  --repo=${test_tmpdir}/ostree-srv/gnomerepo commit -b main --gpg-sign ${TEST_GPG_KEYID_1} --gpg-homedir ${test_tmpdir}/gpghome workdir
+${CMD_PREFIX} ostree --repo=${test_tmpdir}/ostree-srv/gnomerepo static-delta generate main
+${CMD_PREFIX} ostree --repo=${test_tmpdir}/ostree-srv/gnomerepo summary -u --gpg-sign ${TEST_GPG_KEYID_1} --gpg-homedir ${test_tmpdir}/gpghome
+${OSTREE} pull --require-static-deltas R1:main
+
+echo "ok gpg trusted signed commit for delta upgrades"
+
 libtest_cleanup_gpg